-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Improve dashboard performance by decreasing rerenders #30958
base: master
Are you sure you want to change the base?
Conversation
These PRs from Kamil always make me soo happy! ❤️ |
/testenv up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my best to find anything to comment or even nit about in this PR, but this looks really solid. I will have to resort to testing to see there's anything that looks out of the ordinary. Will approve once I'm done with the testing rounds..
@villebro Ephemeral environment spinning up at http://52.38.80.189:8080. Credentials are |
</FiltersPanel> | ||
); | ||
}} | ||
{renderChild} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
||
const toggleDashboardFiltersOpen = useCallback((visible?: boolean) => { | ||
setDashboardFiltersOpen(prevState => visible ?? !prevState); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
], | ||
(metadata, dashboardId, colorScheme, filters, dataMask) => { | ||
// Building nativeFilters object without spreading in reduce | ||
const optimizedNativeFilters = Object.keys(filters).reduce((acc, key) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement. Nit: should this be named nativeFilters
outside of the context of this PR, I don't think "optimized" make sense.
c7aa867
to
c8d5eca
Compare
/testenv up |
@villebro Ephemeral environment spinning up at http://35.91.239.242:8080. Credentials are |
@villebro unfortunately the ephemeral environments have been broken for some time now 🙁 |
Neat AI summary provided by @rusackas 🙂 Key changes include:
The main potential risk areas to watch:
Recommend thorough testing of: |
555df32
to
26a1163
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this! Code looks good but it's a gigantic PR that needs more than a first-pass code review. I'd suggest splitting this into smaller pieces if possible to help with functional tests.
unsetFocusedFilterField, | ||
} from '../../actions/dashboardState'; | ||
import { changeFilter } from '../../actions/dashboardFilters'; | ||
import { refreshChart } from '../../../components/Chart/chartAction'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use absolute paths?
SUMMARY
Due to many inefficient redux selectors and other factors causing numerous rerenders, large dashboards were running very slow.
This PR refactors many components related to Dashboard and native filters - some components are rewritten to functional to utilize React's memoization, Redux selectors optimized to select smaller objects that change less frequently, optimized frequently called functions.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
The tests were conducted on a large dashboard - 300+ charts, 40+ filters. Test steps: reload the page, let the 1st tab load, switch to a different tab, let everything load.
Tested on my M2 Pro Macbook - since it's likely that an average Superset user works on a slower computer, I also run the tests with 4x and 20x CPU throttling.
Testing tool - chrome dev tools, performance tab
No throttling
Before - total scripting time 2.73s (screenshot 1), each chart holder blocks the main thread for ~70ms
After - total scripting time 1.48s (screenshot 2), each chart holder blocks the main thread for ~5ms
4x throttling
Before - total scripting time 11.93s (screenshot 3), each chart holder blocks the main thread for ~300ms
After - total scripting time 6.23s (screenshot 4), each chart holder blocks the main thread for ~20ms
20x throttling
Before - total scripting time 82s (screenshot 5), each chart holder blocks the main thread for ~2s
After - total scripting time 32s (screenshot 6), each chart holder blocks the main thread for ~100ms
SCREENSHOTS BEFORE:
SCREENSHOTS AFTER:
TESTING INSTRUCTIONS
No functional changes - all dashboard functionalities should work like before.
Recommend thorough testing of:
ADDITIONAL INFORMATION